Initial Properties FFI #1269
Initial Properties FFI #1269
Conversation
| /// The [`ICU4XUnicodeScriptMapProperty`], if creation was successful. | ||
| pub data: Option<Box<ICU4XUnicodeScriptMapProperty>>, | ||
| /// Whether creating the [`ICU4XUnicodeScriptMapProperty`] was successful. | ||
| pub success: bool, |
There was a problem hiding this comment.
thought (nb): Tbh I feel like the success is redundant if data is an Option.
I eventually want to use Results here but I'd like to fix some things in Diplomat first.
There was a problem hiding this comment.
I copied this from one of the other components (I think FixedDecimalFormat). I do absolutely think we need to invest more in our Result story. But I don't want to diverge from what we are doing elsewhere until we do it everywhere.
| @@ -0,0 +1,28 @@ | |||
| #ifndef ICU4XUnicodeScriptMapProperty_H | |||
There was a problem hiding this comment.
(thought)
This file is auto-generated, right?
Would it be possible to have Diplomat add a "this file was autogenerated by ..." comment at the top of generated files, to make it easier for reviewers / readers of the code?
There was a problem hiding this comment.
Yes. That's a good idea. rust-diplomat/diplomat#100
| @@ -0,0 +1,24 @@ | |||
| #ifndef ICU4XUnicodeScriptMapPropertyResult_H | |||
There was a problem hiding this comment.
Is there a reason to have this in a separate file from ICU4XUnicodeScriptMapProperty.h, or is it just an implementation detail of Diplomat?
There was a problem hiding this comment.
Implementation detail of Diplomat.
| /** | ||
| * Gets a set for Unicode property ascii_hex_digit from a [`ICU4XDataProvider`]. | ||
| * See [the Rust docs](https://unicode-org.github.io/icu4x-docs/doc/icu_properties/sets/fn.get_ascii_hex_digit.html) for more information. | ||
| */ |
There was a problem hiding this comment.
Is it intentional for the C++ headers to include comments, but not the C headers? It makes sense that the copies of the C headers in ffi/diplomat/cpp are just an implementation detail of the C++ headers, but the same argument doesn't seem to hold for ffi/diplomat/c.
There was a problem hiding this comment.
@gregtatum added C++ docs generation in rust-diplomat/diplomat#85. The primary documentation is intended to be the *.rst files, but perhaps we should put the docs into the *.h files as well. rust-diplomat/diplomat#101
ffi/diplomat/src/properties_maps.rs
Outdated
| } | ||
|
|
||
| impl ICU4XUnicodeScriptMapProperty { | ||
| /// Gets a set for Unicode property ascii_hex_digit from a [`ICU4XDataProvider`]. |
There was a problem hiding this comment.
Nit: This comment and the one for try_get_from_static below seem to incorrectly specify "ascii_hex_digit"?
|
So the binary is slightly different (slightly larger) after changing to use
The good news is that all of these functions are small, and an optimized build should inline most of them, mitigating binary size impact. There are no trait objects or extra standard library stuff that is being pulled in (as expected). So I'm not going to block this change on the code size diff. |
|
I intend to merge this as soon as CI comes back green. @iainireland left a non-blocking review last week, and I integrated the suggestions from that review, and you're welcome to add more post-submit feedback. |
Progress on #1159
I added an FFI for one binary property and one enumerated property.
For the enumerated property, I'm not quite sure what to do about the type signature and return type. Since we return a (wrapper around a)
DataPayload<T>, I think I need to make a unique FFI type for every enumerated property we support. (I would like to get away from this after #1262 is done.) And for now I made it return au32for the script, but should I put the newtype into FFI and make it return the newtype instead?Also, is there a way to avoid duplicating the giant list of script constants into the FFI file?